-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PR: Improve update UX and automatically download latest installers #18619
Conversation
Hello @jsbautista! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2022-09-09 15:07:58 UTC |
@dalthviz ; A preview of the process of checking for new updates: |
@jsbautista just in case, I think the tests are failling since for the new |
Seems like there are some errors with the status constants @jsbautista. To check the tracebacks you can check the CI log: https://github.com/spyder-ide/spyder/runs/7567303687?check_suite_focus=true#step:15:1790 |
@jsbautista I think that this needs a rebase on top of the latest 5.x to prevent errors in the CI. For that fetch in your local repo
If that works without issue the you can force push the rebase with:
If you need help or some error message appears let me know 👍 |
d95f5ea
to
c6bc21f
Compare
Hi @dalthviz, could you give me a last review before removing the WIP? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jsbautista thanks for all the work on this! I left some suggestions and comments. I think the most important part to check is the location of the installation thread logic. Checking the code a little bit more seems to me that the status of the installation should be handle by the installation widget and we need to provide methods so the container can call some start method using the status widget (so we have a relation like container -> status widget -> installation widget).
If you have any questions or comments maybe we can do a pair programming session to test the ideas or check if there are more elements to take into account to handle better the interactions between container, status widget and installation widget.
Let me know!
Hi @dalthviz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the work @jsbautista ! I think this is getting closer and closer to be finished 👍
I left some suggestions regarding the calls to set the status and also some remaning logic position and status values suggestions (like using the Spyder version for the NO_STATUS
constant). Besides that there are some PEP8 issue to be fixed.
Also, could you update the gif to preview the way this is working, please?
Thanks again! If you need anything to address the review, questions or comments, let me know!
@dalthviz ; A preview of the process of checking for new updates: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @jsbautista! This looks pretty good!! Most of my suggestions are style issues, otherwise looks almost ready to me.
As a recommendation, please don't mark reviewer comments as solved. That's the work of reviewers, after checking that you correctly addressed their comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jsbautista, last comments from my side.
I've made a suggestion to fix the description messages, FYI |
Co-authored-by: Daniel Althviz Moré <d.althviz10@uniandes.edu.co>
Also, just to confirm @dalthviz @ccordoba12 once this is ready, this should either have a major rebase to consolidate the many "WIP" commits into atomic, well-described units before merging, or just be squash merged. |
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@CAM-Gerlach this will be squash merged 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jsbautista ! Left some comments regarding some issues raising when testing the latest changes.
Also, just in case, a preview for others to see how this will be working now to reuse a previously downloaded installer executable:
- The installer executable was downloaded in a previous session but not installed and the check update dialog appears:
- In case the installer was downloaded in the current session or a previous one but the user decided to not proceed with the installation update (in case of the Windows installer the user choosed to not close Spyder for example or choosed
No
in the check update dialog):
reply.setStandardButtons(QMessageBox.Yes | QMessageBox.No) | ||
reply.exec_() | ||
if reply.result() == QMessageBox.Yes: | ||
self.start_installation_update() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is triggering an TypeError
exception since the latest_release_version
arg is not being passed:
Traceback (most recent call last):
File "C:\Users\dalth\AppData\Local\Programs\Spyder\pkgs\spyder\plugins\application\widgets\status.py", line 111, in show_installation_dialog_or_menu
self.installer.continue_install()
File "C:\Users\dalth\AppData\Local\Programs\Spyder\pkgs\spyder\plugins\application\widgets\install.py", line 191, in continue_install
self.start_installation_update()
TypeError: start_installation_update() missing 1 required positional argument: 'latest_release_version'
I think we should, from the container, pass the latest_release_version
to the status widget/installer widget even if the user doesn't accept immediatly the first dialog to update (the one that informs that a new version is available)
Co-authored-by: Daniel Althviz Moré <d.althviz10@uniandes.edu.co>
…into 5.xUpdateSpyder
Co-authored-by: Daniel Althviz Moré <d.althviz10@uniandes.edu.co> Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Just in case @jsbautista seems like the error to generated the Windows installer is unrelated to the changes on the PR but to the PyPI json API cache to retrieve a release/info about the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the changes @jsbautista ! I left a simple comment for the latest changes regarding a variable name.
Also, as a suggestion we were discussing with the others, we should leave the update related status bar widget to the left of the rest of status bar widgets. Checking, for that we will need to add some entries in a couple of iterables in the status plugin. Basically, we need change the set and list at:
spyder/spyder/plugins/statusbar/plugin.py
Lines 49 to 52 in 38e3adf
INTERNAL_WIDGETS_IDS = { | |
'clock_status', 'cpu_status', 'memory_status', 'read_write_status', | |
'eol_status', 'encoding_status', 'cursor_position_status', | |
'vcs_status', 'lsp_status', 'kite_status', 'completion_status'} |
And
spyder/spyder/plugins/statusbar/plugin.py
Lines 216 to 219 in 38e3adf
internal_layout = [ | |
'clock_status', 'cpu_status', 'memory_status', 'read_write_status', | |
'eol_status', 'encoding_status', 'cursor_position_status', | |
'vcs_status', 'lsp_status', 'kite_status', 'completion_status'] |
Adding to them the following entries: 'interpreter_status', 'application_update_status'
With that the status widget for the updates will be shown at the left of the others:
If you have any question or comment let me know!
Co-authored-by: Daniel Althviz Moré <d.althviz10@uniandes.edu.co>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jsbautista ! I think this is ready for a squash merge!
Do you have any other comments @mrclary @CAM-Gerlach @ccordoba12 ?
I'm good |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to look good from a UX and UI text perspective, with some improvements to follow in a followup PR. I haven't had the time to fully QA test it myself on a Windows machine, but it seems others have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jsbautista for your hard work on adding this new functionality! It's a terrific improvement!!
PR: Improve update UX and automatically download latest installers
Description of Changes
Part of #19112
Affirmation
By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.
I certify the above statement is true and correct: @jsbautista